Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update deployment_id in ML trained model deployment start request #2325

Merged
merged 5 commits into from
Nov 6, 2023

Conversation

dolaru
Copy link
Member

@dolaru dolaru commented Nov 1, 2023

This adds the missing deployment_id query parameter to the _start request, that was introduced in v8.8.0.

Docs say:

deployment_id
(Optional, string) A unique identifier for the deployment of the model.

@dolaru dolaru requested a review from a team as a code owner November 1, 2023 17:35
@dolaru dolaru self-assigned this Nov 1, 2023
@dolaru dolaru requested a review from a team as a code owner November 1, 2023 17:35
@dolaru dolaru changed the title Add missing deployment_id parameter in ml.start_trained_model_deployment request Update deployment_id in ML trained model deployment start/stop request Nov 1, 2023
Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM but did you forget to run make contrib ?

@dolaru
Copy link
Member Author

dolaru commented Nov 2, 2023

@davidkyle make contrib is not happy:

Error: The property 'deployment_id' does not exist in the rest-api-spec ml.stop_trained_model_deployment url template
At: /Users/dolaru/workspace/elasticsearch-specification/specification/ml/stop_trained_model_deployment/MlStopTrainedModelDeploymentRequest.ts:30:47

29   */
30  export interface Request extends RequestBase {
31    path_parts: {
32      /**
33       * The unique identifier for the deployment of the model.
34       */
35      deployment_id: Id
36    }
37    query_parameters: {
38      /**

Looks like the JSON spec is not up to date and I don't know what's the proper way to update it. 😞

@dolaru dolaru requested a review from davidkyle November 2, 2023 16:40
@dolaru dolaru changed the title Update deployment_id in ML trained model deployment start/stop request [DO NOT MERGE] Update deployment_id in ML trained model deployment start/stop request Nov 2, 2023
@dolaru dolaru marked this pull request as draft November 2, 2023 16:43
@dolaru dolaru changed the title [DO NOT MERGE] Update deployment_id in ML trained model deployment start/stop request Update deployment_id in ML trained model deployment start/stop request Nov 2, 2023
@pquentin pquentin requested a review from a team November 6, 2023 07:43
@pquentin
Copy link
Member

pquentin commented Nov 6, 2023

Hey @dolaru, the check is complaining that https://github.com/elastic/elasticsearch/blob/main/rest-api-spec/src/main/resources/rest-api-spec/api/ml.stop_trained_model_deployment.json does not contain deployment_id. I believe model_id should be changed to deployment_id there first?

I'm also wondering if the same applies to ml.start_trained_model_deployment, but in that case there is a deployment_id described as "The Id of the new deployment. Defaults to the model_id if not set.".

@dolaru
Copy link
Member Author

dolaru commented Nov 6, 2023

the check is complaining that https://github.com/elastic/elasticsearch/blob/main/rest-api-spec/src/main/resources/rest-api-spec/api/ml.stop_trained_model_deployment.json does not contain deployment_id. I believe model_id should be changed to deployment_id there first?

@davidkyle can I leave this with you please?

I'm also wondering if the same applies to ml.start_trained_model_deployment

@pquentin for the _start API, model_id is a required path parameter and deployment_id is an optional query parameter that can be used if multiple deployments from the same trained model are required. For example:

# Two separate deployments of the ELSER v2 model
POST _ml/trained_models/.elser_model_2/deployment/_start?deployment_id=elser_main
POST _ml/trained_models/.elser_model_2/deployment/_start?deployment_id=elser_secondary

For the _stop API, the path parameter should be deployment_id since it targets a trained model deployment, not a model definition. This will introduce a breaking change in clients since the named model_id parameter would no longer be accepted - I think it's something we'll have to live with.

@dolaru dolaru force-pushed the missing-ml-start-tm-param branch from 03ad6b0 to 4b2da88 Compare November 6, 2023 12:53
@davidkyle
Copy link
Member

For the _stop API, the path parameter should be deployment_id

This is correct but if renaming the path parameter breaks the client I think we should rely on documentation to state it should be the deployment Id. @pquentin is it possible to deprecate the model_id usage and add a param deployment_id?

The change to the start deployment API is good and necessary as without it a user cannot deploy a model multiple times.

@dolaru do you want to take the Stop API change out of this PR and just merge the Start API change? That way we have made some progress while we figure out the stop issue.

@dolaru dolaru marked this pull request as ready for review November 6, 2023 14:29
@dolaru dolaru changed the title Update deployment_id in ML trained model deployment start/stop request Update deployment_id in ML trained model deployment start request Nov 6, 2023
@dolaru
Copy link
Member Author

dolaru commented Nov 6, 2023

take the Stop API change out of this PR

Done ✅

Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I don't know why the elasticsearch-serverless-openapi.json change has appeared in this PR

@dolaru
Copy link
Member Author

dolaru commented Nov 6, 2023

I don't know why the elasticsearch-serverless-openapi.json change has appeared in this PR

Probably because I didn't run make contrib on the vocabulary scores PR. 😅
I'll take that out since it's not related to this PR.

Copy link
Contributor

github-actions bot commented Nov 6, 2023

Following you can find the validation results for the API you have changed.

API Status Request Response
ml.start_trained_model_deployment Missing test Missing test

You can validate this API yourself by using the make validate target.

@dolaru dolaru merged commit e627f16 into main Nov 6, 2023
6 checks passed
@dolaru dolaru deleted the missing-ml-start-tm-param branch November 6, 2023 16:09
github-actions bot pushed a commit that referenced this pull request Nov 6, 2023
…2325)

This adds the missing `deployment_id` query parameter to the `_start` request, that was introduced in v8.8.0.

[Docs](https://www.elastic.co/guide/en/elasticsearch/reference/8.8/start-trained-model-deployment.html#start-trained-model-deployment-query-params) say:

> deployment_id
> (Optional, string) A unique identifier for the deployment of the model.

(cherry picked from commit e627f16)
github-actions bot pushed a commit that referenced this pull request Nov 6, 2023
…2325)

This adds the missing `deployment_id` query parameter to the `_start` request, that was introduced in v8.8.0.

[Docs](https://www.elastic.co/guide/en/elasticsearch/reference/8.8/start-trained-model-deployment.html#start-trained-model-deployment-query-params) say:

> deployment_id
> (Optional, string) A unique identifier for the deployment of the model.

(cherry picked from commit e627f16)
github-actions bot pushed a commit that referenced this pull request Nov 6, 2023
…2325)

This adds the missing `deployment_id` query parameter to the `_start` request, that was introduced in v8.8.0.

[Docs](https://www.elastic.co/guide/en/elasticsearch/reference/8.8/start-trained-model-deployment.html#start-trained-model-deployment-query-params) say:

> deployment_id
> (Optional, string) A unique identifier for the deployment of the model.

(cherry picked from commit e627f16)
github-actions bot pushed a commit that referenced this pull request Nov 6, 2023
…2325)

This adds the missing `deployment_id` query parameter to the `_start` request, that was introduced in v8.8.0.

[Docs](https://www.elastic.co/guide/en/elasticsearch/reference/8.8/start-trained-model-deployment.html#start-trained-model-deployment-query-params) say:

> deployment_id
> (Optional, string) A unique identifier for the deployment of the model.

(cherry picked from commit e627f16)
davidkyle pushed a commit that referenced this pull request Nov 15, 2023
davidkyle pushed a commit that referenced this pull request Nov 15, 2023
davidkyle pushed a commit that referenced this pull request Nov 15, 2023
davidkyle pushed a commit that referenced this pull request Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants